Skip to content

Conversation

TracerDS
Copy link
Contributor

@TracerDS TracerDS commented May 24, 2023

@tederis
Copy link
Member

tederis commented May 24, 2023

I think that the length of these constants makes them cumbersome and difficult to use. I doubt that long constructions like constants.VehicleLightOverride.Disable can be usefull. I would suggest to make them shorter.

@TracerDS TracerDS marked this pull request as ready for review May 24, 2023 13:32
@tederis
Copy link
Member

tederis commented May 24, 2023

In addition, I would suggest to make the table read-only.

Copy link

@CrosRoad95 CrosRoad95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me, just maybe other people may have concern about names, but apart of that it is good to go

@lopezloo lopezloo added the enhancement New feature or request label May 24, 2023
@patrikjuvonen patrikjuvonen changed the title New Lua constants - #3022 Fix #3022: New Lua constants May 25, 2023
Copy link
Contributor

@Pirulax Pirulax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, whats the point of not directly assigning the values to constants.* but instead making a local var first?
If there's no good reason, then we should just assign it directly [this also reduces the chance of interfering with existing scripts]

Also, there's a chance that some scripts might break if these constants are loaded into every script, we should have a meta.xml toggle for it, that toggles this feature per-resource [as theres one LuaVM per resource]

Also... I'm not sure if constants is the solution.
Perhaps we should just push them into the global namespace? [Like KeyStates.Up rather than constants.KeyStates.up.]
[I'm sure some people would prefer the one over the other]

And, as @tederis noted, making the tables read only might be useful [though Lua is somewhat limited in that regard]

@Pirulax
Copy link
Contributor

Pirulax commented Jan 6, 2024

Also something I just realized, these constants are pretty useless on their own.
There should be a way to convert to/from name/value.
Given:

local MyEnum = {
    First = 1
}

MyEnum['First'] should return 1. MyEnuim(1) should return First
This is similar to how Python work (Though Python returns special EnumValue objects that we can't afford in terms of performance, so we're going with the simpler solution here).

Tables are now read only;
Better value handling
@TracerDS
Copy link
Contributor Author

I think that the length of these constants makes them cumbersome and difficult to use. I doubt that long constructions like constants.VehicleLightOverride.Disable can be usefull. I would suggest to make them shorter.

What would we rename them to? 🤔

Also something I just realized, these constants are pretty useless on their own. There should be a way to convert to/from name/value. Given:

local MyEnum = {
    First = 1
}

MyEnum['First'] should return 1. MyEnuim(1) should return First This is similar to how Python work (Though Python returns special EnumValue objects that we can't afford in terms of performance, so we're going with the simpler solution here).

Implemented. Thanks for the suggestion ❤

@TracerDS TracerDS changed the title Fix #3022: New Lua constants New Lua constants (#3022) Jun 1, 2024
@TracerDS
Copy link
Contributor Author

Im not interested in advancing this PR further anymore.
If someone wants to refresh it, feel free to do so.

@TracerDS TracerDS closed this Dec 23, 2024
@TracerDS TracerDS deleted the 210523_NewConstants branch December 23, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants